🚅 Deployed to the rivet-pr-5010 environment in rivet-frontend

Service	Status	Web	Updated (UTC)
frontend-inspector	🕒 Building (View Logs)	Web	May 19, 2026 at 8:12 pm
frontend-cloud	🕐 Queued (View Logs)	Web	May 19, 2026 at 8:11 pm
kitchen-sink	🕐 Queued (View Logs)	Web	May 19, 2026 at 8:11 pm
website	😴 Sleeping (View Logs)	Web	May 19, 2026 at 4:44 pm
ladle	❌ Build Failed (View Logs)	Web	May 9, 2026 at 3:40 pm
mcp-hub	✅ Success (View Logs)	Web	May 9, 2026 at 3:40 pm

• fix(workflow-engine): only commit step state after success #5010 : 2 dependent PRs (#5052 , #5058 ) 👈 (View in Graphite)
• fix(rivetkit): bind methods through createWriteThroughProxy #4987
• fix(rivetkit): remove client-side connection request size check #5057
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Code Review: fix(workflow-engine): only commit step state after success

Note: This PR is still in Draft status with an empty description/checklist.

Overview

This PR fixes a correctness bug in workflow-engine where actor state mutations inside a failing step were being persisted even though the step needs to be retried. On retry, the step would execute against incorrectly mutated state. The fix:

1. Introduces RAW_STATE_SYMBOL to bypass the write-through proxy and read raw actor state.
2. Wraps every step() and tryStep() call in #withActorAccessAndStateRollback, which snapshots state+vars before the step and rolls them back on any error.
3. Removes the per-branch flushStorage() calls in step failure catch blocks, consolidating to the single flush in setRetryState / executeWorkflow's unrecoverable-error path.
4. Removes the stepData.error fallback (justified by the new test).

The core idea is correct and addresses a real workflow replay bug.

Bug: Breaks stateless actors using workflow steps

withActorAccessAndStateRollback unconditionally calls this.#runCtx[RAW_STATE_SYMBOL]() (workflow/context.ts:618), but ActorContextHandleAdapter[RAW_STATE_SYMBOL] throws stateNotEnabledError() when the actor has no state configured (native.ts:2460-2465).

This means any actor that uses workflow steps but does not have a state field will crash on the snapshot call, breaking all ctx.step() invocations regardless of whether the step touches state. The guard needs to handle the state-disabled case, for example by returning undefined and skipping the rollback assignment when #stateEnabled is false.

Concern: structuredClone(vars) can throw for non-cloneable objects

vars are runtime-in-memory (not persisted), so they can legitimately hold class instances, Promises, or WeakRefs. structuredClone throws a DataCloneError on these, crashing the snapshot before the step runs. Worth documenting that vars must be structured-cloneable when workflows are used, or guarding the clone.

Minor: Notification fires before flush for timeout/critical errors

Previously flushStorage() was called before notifyStepError for StepTimeoutError/CriticalError. Now the notify fires before the eventual top-level flush in executeWorkflow. This slightly widens the crash window between notification and persistence (duplicate notification possible on replay). Not a durability regression, but worth a comment.

Code Quality

Positive changes:

• Consolidating entry.kind.data.error and entry.dirty = true before all the if-branches eliminates three identical blocks.
• Removing the double-flush on failure (catch block plus setRetryState) is correct; the top-level flush in executeWorkflow guarantees persistence on all error paths.
• Removing the stepData.error fallback is justified since the new test confirms the error is always written to the step entry.

Issues:

1. Misleading test name (steps.test.ts:573): 'should not commit step error data to entry on failure' but the test asserts expect(entry.kind.data.error).toBe('Error: step failed'), i.e. the error is committed. Rename to 'should commit error but not output to step entry on failure'.

2. Bare catch {} in the new test's first-run block silently swallows unexpected errors. Add a comment explaining what error is expected.

3. // @ts-nocheck in workflow/context.ts: the RAW_STATE_SYMBOL call is unverified by the type system. The interface declares the method returning TState, with TState = never for stateless actors the type system would surface the mismatch if checking were enabled.

4. NativeConnAdapter[RAW_STATE_SYMBOL] (native.ts:1162): added for interface completeness. The workflow rollback path only flows through ActorContextHandleAdapter, so this is dead code in practice. Fine as-is.

Test Coverage

• New test is a good regression guard for the flush behavior; see naming concern above.
• Removed StripStepHistoryErrorDriver test is a coherent removal - it tested the stepData.error fallback that was deliberately deleted.
• Missing: no test covers stateless actor plus workflow steps. Once the crash is fixed, a test like 'should not crash when actor has no state and step fails' would prevent regression.
• Loop expectation change (toBe(2) to toBe(3)) appears correct: crash at iteration 3, state now only persisted after successful Loop.continue at end of iteration 2, so iterations 3-5 need to re-execute on resume.

Summary

The core approach is sound. Please address the stateless actor crash (and consider the vars clone concern) before merging.

Merge activity

• May 19, 8:06 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 19, 8:08 PM UTC: The Graphite merge of this pull request was cancelled.
• May 19, 8:08 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 19, 8:12 PM UTC: Graphite rebased this pull request as part of a merge.
• May 19, 8:13 PM UTC: @abcxff merged this pull request with Graphite.